Review: feat(cli): multi-provider skill installation

Clean, well-structured work with thorough test coverage. The separation of concerns is good (providers.ts for resolution, config-manager for persistence, provider.ts for CLI handlers). The prototype-pollution fix (in → Object.hasOwn) is a real bug catch. A few items worth addressing before merge.

1. setProvider relies on mutable reference aliasing

File: packages/cli/src/utils/config-manager.ts
Severity: Low

setProvider(name: string): void {
  const config = this.loadConfig();
  config.provider = name;
  this.saveConfig();
}

loadConfig() returns the config object and caches it on this.config. Mutating the return value works because config and this.config are the same reference. If loadConfig() ever returned a copy (e.g., for immutability), this would silently break — saveConfig() would write the stale cached version without the new provider field.

Recommendation: Mutate this.config directly for clarity.

setProvider(name: string): void {
    this.loadConfig();
    this.config!.provider = name;
    this.saveConfig();
  }

Worth noting: every other setter in the class uses the same implicit-alias pattern (setRegistryUrl, setPackageConfigValue, clearPackageConfig, clearPackageConfigValue). Probably worth changing all of them to this pattern as well.

2. Detection via parent directory may cause false positives

File: packages/cli/src/utils/providers.ts — detectProviders()
Severity: Medium

detectProviders checks for the existence of parent directories like ~/.cursor, ~/.config/agents, etc. On typical developer machines:

• ~/.claude exists on any machine with Claude Code installed, even if the user doesn't want to target it for mpak.
• ~/.config/agents or ~/.config/opencode may exist for unrelated reasons.
• Most providers' parent dirs (~/.cursor, ~/.copilot) are created by those tools for config/state regardless of skill support.

This means users with multiple coding tools installed will frequently hit the "Multiple providers detected" error and be forced to run mpak provider set before they can install anything.

Recommendation: Consider checking for the skills directory itself, or a tool-specific config file, rather than just the parent directory.

3. handleProviderShow test doesn't verify execution stops after process.exit

File: packages/cli/src/commands/provider.test.ts
Severity: Low

The handleProviderShow error test mocks process.exit as:

vi.spyOn(process, "exit").mockImplementation(() => undefined as never);

This mock returns silently, so the function continues executing after process.exit(1). The test asserts exitSpy was called but doesn't verify the function actually stopped. In contrast, the handleProviderSet test correctly uses a throwing mock:

vi.spyOn(process, "exit").mockImplementation(() => { throw exitError; });

Recommendation: Make the handleProviderShow test use a throwing mock for consistency.

4. Copilot and Gemini skills directory paths may be incorrect

File: packages/cli/src/utils/providers.ts — PROVIDERS map
Severity: Medium

• Copilot: Mapped to ~/.copilot/skills. GitHub Copilot doesn't use ~/.copilot as its config directory — it typically uses ~/.config/github-copilot or VS Code's extension settings.
• Gemini: Mapped to ~/.gemini/skills. Google's Gemini CLI uses ~/.gemini but it's unclear whether it supports a skills/ subdirectory convention.

Recommendation: Verify these paths against actual tool documentation or behavior. If aspirational/placeholder, document that explicitly.

5. No --provider flag on skill search — rationale undocumented

File: packages/cli/src/program.ts
Severity: Informational

The test explicitly asserts skill search does NOT accept --provider, which makes sense since search hits the registry. But users may expect symmetry across all skill subcommands.

Recommendation: Add a brief code comment or CLI help text explaining why search doesn't accept --provider.

Minor / Nits

A. PROVIDER_PARENTS duplicates PROVIDERS logic — Both maps hardcode closely related paths. The parent could be derived from the skills dir using path.dirname(), reducing the risk of drift. The structural invariant test mitigates this, but DRY is still preferable.

B. setProvider validate-on-read design undocumented in storage layer — setProvider accepts arbitrary strings with no validation against known provider names. Validation is deferred to resolveProvider() on read — a reasonable design, documented in the test file ("should accept arbitrary strings"). However, config-manager.ts itself has no comment explaining why validation is absent. A one-liner on setProvider (e.g., // No validation — resolveProvider() validates on read) would help future contributors.

C. Empty string edge case is internal-only — The test "treats empty string explicit as no explicit" is good defensive coverage, but Commander will never pass "" for --provider (it requires a value). Worth a brief comment noting this exercises an internal API contract.

What's Good

• Prototype-pollution fix: in → Object.hasOwn for isValidProvider — "constructor", "toString" etc. would have returned true with in.
• Structural invariant tests: Verifying every provider in PROVIDERS has a corresponding PROVIDER_PARENTS entry, and that all skills dirs end in /skills under $HOME — excellent for catching drift.
• Per-layer validation tests: Each resolution layer (explicit, env, config) validates independently with source-specific error messages.
• Error UX: The ambiguity error suggests both mpak provider set and --provider as remediation.
• Resolution priority chain: The 4-level priority (flag > env > config > auto-detect) is well-designed and clearly documented.

Requesting changes based on the findings in the review comment above.

APPROVE WITH COMMENTS

Well-structured provider abstraction with thorough test coverage. The Object.hasOwn fix, goose detection change, and ambiguous-detection-defaults-to-claude behavior are all solid. A few suggestions:

• Test coverage gap (non-blocking): install.ts and list.ts now call resolveProvider(options.provider), but no tests verify the --provider option flows end-to-end from CLI handler to resolution. program.test.ts covers flag registration and providers.test.ts covers resolution in isolation, but the wiring between them isn't tested. Worth adding if you're doing another pass.
• Observability (nitpick): mpak provider list shows detected providers but not the active resolved provider. mpak provider show identifies the provider but not which resolution tier selected it (env, config, auto-detect). Showing the source (e.g., cursor (via MPAK_PROVIDER)) in show, and an Active: footer in list, would close the debugging loop.
• Goose path divergence (nitpick): A brief comment in the PROVIDERS map noting that goose's skills dir (~/.config/agents/skills) intentionally differs from its detection parent (~/.config/goose) would help future maintainers.